Remove Angular from visualize #20295

ppisljar · 2018-06-28T11:27:57Z

removes angular from visualize directive and turns it into a class.

From now on the data fetching (that was earlier part of the <visualize> directive) will be triggered by the visualize loader directly, which then renders or updates the Visualization React component with the fetched data.

This PR removes Angular completely from out central rendering infrastructure 🎉

elasticmachine · 2018-07-05T07:41:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-05T09:18:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-05T11:06:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-05T12:35:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

markov00

LGTM
Tested on chrome/osx.
Just a minor comment

markov00 · 2018-07-06T08:36:19Z

src/ui/public/visualize/loader/embedded_visualize_handler.js

    this._listeners = new EventEmitter();
    // Listen to the first RENDER_COMPLETE_EVENT to resolve this promise
    this._firstRenderComplete = new Promise(resolve => {
      this._listeners.once(RENDER_COMPLETE_EVENT, resolve);
    });
-    this._element.on('renderComplete', () => {
+    this._element.addEventListener('renderComplete', () => {


Seems that we are not removing the event listener of this renderComplete event in the destroy. Is it fine?

I would agree, I think we should deregister this properly.

timroes · 2018-07-06T08:46:49Z

src/ui/public/visualize/index.js

@@ -17,4 +17,4 @@
 * under the License.
 */

-import './visualize';
+import './loader';


That doesn't make much sense, since loader itself just exports some classes. We should either put an export * from './loader'; here in case we want to enable importing getVisualizeLoader from ui/visualize directly, or just remove the whole index.js file.

src/ui/public/visualize/loader/embedded_visualize_handler.js

      this._listeners.emit(RENDER_COMPLETE_EVENT);
    });
+
+    this._loaded = false;
+    this._destroyed = false;


timroes · 2018-07-06T08:57:53Z

src/ui/public/visualize/loader/embedded_visualize_handler.js

+    });
+  };
+
+  _fetchAndRender = _.debounce((forceFetch) => {


Give this a default value = false for better readability.

timroes · 2018-07-06T09:35:12Z

src/ui/public/visualize/loader/embedded_visualize_handler.js

@@ -17,7 +17,11 @@
 * under the License.
 */

+import _ from 'lodash';


I think we should rather start using named imports for that (since it would potentially allow for better tree shaking - and imho also improves readability): import { debounce } from 'lodash';

timroes · 2018-07-06T09:36:40Z

src/ui/public/visualize/loader/embedded_visualize_handler.js

  }

  /**
   * Destroy the underlying Angular scope of the visualization. This should be
   * called whenever you remove the visualization.
   */
  destroy() {
-    this._scope.$destroy();
+    this._destroyed = true;
+    this._vis.removeListener('reload', this._reloadVis);


I think there might be a tiny chance, that we're actually currently have triggered a fetchAndRender that's still in debounce while we're destroying. I think in that case we should cancel the debounce. It seems the method returned from the debounce call above should have a property cancel so that we can call this._fetchAndRender.cancel() here.

timroes · 2018-07-06T09:37:25Z

src/ui/public/visualize/loader/visualize_data_loader.js

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import _ from 'lodash';


Same here, could we use named imports, please.

timroes · 2018-07-06T09:38:27Z

src/ui/public/visualize/loader/visualize_data_loader.js

+
+    const handlerParams = {
+      ...props,
+      forceFetch: forceFetch,


🎨 Just forceFetch would be enough.

timroes · 2018-07-06T09:39:44Z

src/ui/public/visualize/loader/visualize_data_loader.js

+      this._previousVisState = this._vis.getState();
+      this._previousRequestHandlerResponse = requestHandlerResponse;
+
+      this._visData = canSkipResponseHandler ?


🎨 I would prefer the following version slightly:

if (!canSkipResponseHandler) { this._visData = await Promise.resolve(this._responseHandler(this._vis, requestHandlerResponse)); }

elasticmachine · 2018-07-09T07:58:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-07-09T08:59:04Z

src/ui/public/visualize/loader/visualize_data_loader.js

+    this._responseHandler = getHandler(responseHandlers, responseHandler);
+  }
+
+  fetch = async (props, forceFetch = false) => {


I think that can use the regular function syntax async fetch(props, forceFetch = false) { since we don't seem to call this anywhere in a weird this-overwriting way.

timroes · 2018-07-09T10:36:16Z

src/ui/public/visualize/loader/visualize_data_loader.js

+      this.props.searchSource.cancelQueued();
+      this._vis.requestError = e;
+      if (isTermSizeZeroError(e)) {
+        return toastNotifications.addDanger(


We should really move this to a more sane place (though not necessarily in this PR, but we should start thinking about what a sane place could be).

timroes

The code looks now good for me. Tested it out (Chrome, Linux) with different combinations of visualizations/dashboards/etc couldn't find any issue.

LGTM, as long as it's merged after #20480 has been reviewed, merged, and this one rebased on it.

elasticmachine · 2018-07-09T11:46:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-09T14:40:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-09T19:08:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

LeeDr · 2018-07-23T21:09:03Z

Just a note that this PR apparently broke auto refresh on visualizations and dashboards.

timroes · 2018-07-24T10:55:01Z

@LeeDr see #20813 that was created a week ago, we already marked it as a blocker for 6.4.0 after syncing with Sharing and Discovery about the time picker issues.

ppisljar added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) blocked labels Jun 28, 2018

ppisljar force-pushed the fix/visualizeRemoveAngular branch 5 times, most recently from 723fe1b to 71e3704 Compare July 5, 2018 05:50

ppisljar removed the blocked label Jul 5, 2018

elastic deleted a comment from elasticmachine Jul 5, 2018

ppisljar force-pushed the fix/visualizeRemoveAngular branch from 71e3704 to c0ccec2 Compare July 5, 2018 06:32

ppisljar force-pushed the fix/visualizeRemoveAngular branch 2 times, most recently from 8d1192c to 457345d Compare July 5, 2018 08:12

ppisljar force-pushed the fix/visualizeRemoveAngular branch 2 times, most recently from 0ce3f31 to 3bc7801 Compare July 5, 2018 09:58

ppisljar force-pushed the fix/visualizeRemoveAngular branch 2 times, most recently from b304555 to 266bae8 Compare July 5, 2018 11:29

ppisljar mentioned this pull request Jul 5, 2018

fixing dashboard state filters #20480

Merged

ppisljar force-pushed the fix/visualizeRemoveAngular branch from 266bae8 to 9624da8 Compare July 5, 2018 13:04

ppisljar added the review label Jul 5, 2018

timroes added chore v7.0.0 labels Jul 6, 2018

markov00 approved these changes Jul 6, 2018

View reviewed changes

timroes suggested changes Jul 6, 2018

View reviewed changes

timroes reviewed Jul 9, 2018

View reviewed changes

timroes approved these changes Jul 9, 2018

View reviewed changes

ppisljar added 3 commits July 9, 2018 15:32

removing angular from visualize

dd55ace

applying CJs feedback

15e3f65

updating based on tims suggestions

fb11273

ppisljar force-pushed the fix/visualizeRemoveAngular branch from f236803 to b24c043 Compare July 9, 2018 13:32

removing listener on destroy

3a76eb6

ppisljar force-pushed the fix/visualizeRemoveAngular branch from b24c043 to 3a76eb6 Compare July 9, 2018 17:57

ppisljar merged commit fffa3d4 into elastic:master Jul 9, 2018

ppisljar mentioned this pull request Jul 9, 2018

[6.x] Remove Angular from visualize (#20295) #20593

Merged

ppisljar deleted the fix/visualizeRemoveAngular branch July 9, 2018 20:39

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jul 11, 2018

Remove Angular from visualize (elastic#20295)

c112b53

ppisljar added a commit that referenced this pull request Jul 11, 2018

Remove Angular from visualize (#20295) (#20593)

6c4b4d2

timroes mentioned this pull request Jul 16, 2018

Auto refresh is no longer working #20813

Closed

timroes mentioned this pull request Jul 30, 2018

Fix empty vis toasts #21388

Merged

cjcenizal mentioned this pull request Aug 30, 2018

Timeout screen triggers with autorefresh and tabbing #22466

Closed

timroes mentioned this pull request Sep 5, 2018

Fix broken visualize CSS #22707

Merged

ppisljar restored the fix/visualizeRemoveAngular branch September 26, 2018 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Angular from visualize #20295

Remove Angular from visualize #20295

ppisljar commented Jun 28, 2018 •

edited

Loading

elasticmachine commented Jul 5, 2018

elasticmachine commented Jul 5, 2018

elasticmachine commented Jul 5, 2018

elasticmachine commented Jul 5, 2018

markov00 left a comment

markov00 Jul 6, 2018

timroes Jul 9, 2018

timroes Jul 6, 2018

This comment was marked as resolved.

timroes Jul 6, 2018

timroes Jul 6, 2018

timroes Jul 6, 2018

timroes Jul 6, 2018

timroes Jul 6, 2018

timroes Jul 6, 2018

elasticmachine commented Jul 9, 2018

timroes Jul 9, 2018

timroes Jul 9, 2018

timroes left a comment

elasticmachine commented Jul 9, 2018

elasticmachine commented Jul 9, 2018

elasticmachine commented Jul 9, 2018

LeeDr commented Jul 23, 2018

timroes commented Jul 24, 2018

Remove Angular from visualize #20295

Remove Angular from visualize #20295

Conversation

ppisljar commented Jun 28, 2018 • edited Loading

elasticmachine commented Jul 5, 2018

💔 Build Failed

elasticmachine commented Jul 5, 2018

💔 Build Failed

elasticmachine commented Jul 5, 2018

💔 Build Failed

elasticmachine commented Jul 5, 2018

💔 Build Failed

markov00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 9, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 9, 2018

💔 Build Failed

elasticmachine commented Jul 9, 2018

💔 Build Failed

elasticmachine commented Jul 9, 2018

💚 Build Succeeded

LeeDr commented Jul 23, 2018

timroes commented Jul 24, 2018

ppisljar commented Jun 28, 2018 •

edited

Loading